Update the negotiation and Locale logic to match latest algo#335
Conversation
|
it removes the |
|
Fixes #334 |
| const language = result[1] || undefined; | ||
| const script = result[2] || undefined; | ||
| const region = result[3] || undefined; | ||
| const variant = result[4] || undefined; |
There was a problem hiding this comment.
This can be written more succinctly as: let [, language, script, region, variant] = result.
| this.language = language.toLowerCase(); | ||
| } | ||
| if (script) { | ||
| this.script = script[0].toUpperCase() + script.substr(1); |
There was a problem hiding this comment.
Please use slice or substring, as substr is considered a legacy function.
| "ta": "ta-taml-in", | ||
| "uk": "uk-cyrl-ua", | ||
| "zh": "zh-hans-cn", | ||
| "zh-hant": "zh-Hant-TW", |
There was a problem hiding this comment.
Please be consistent with letter case, or comment on the reason why it needs to be inconsistent if that's the case.
|
|
||
| const availLocales = | ||
| new Set(availableLocales.map(locale => new Locale(locale, true))); | ||
| const availLocales = new Map(); |
There was a problem hiding this comment.
Let's take the opportunity to fix the cryptic naming here. availableLocales vs availLocales hides the difference between the two. I suggest availableLanguageTags and availableLanguagesByTag, respectively.
| break; | ||
| } | ||
| } | ||
| for (const key of availLocales.keys()) { |
There was a problem hiding this comment.
Maybe rename key to tag? (Here and elsewhere.)
| this[part].toLowerCase() === locale[part].toLowerCase()); | ||
| return ((thisRange && this[part] === undefined) || | ||
| (otherRange && locale[part] === undefined) || | ||
| this[part] === locale[part]); |
There was a problem hiding this comment.
Understanding this method is important for understanding the whole algorithm and I'm not fond of adding two boolean arguments to it. It's hard to follow what they mean when the method is called. In fact, I think I largely preferred the old code which used an explicit * to represent ranges.
A few suggestions on how to improve this code. I'm not saying all of them need to be applied at the same time, but I'd like to see this method improved somehow.
- Remove the
thisRangeparam as it is alwaystrueacross all callsites, or start using it for exact matching. - Use an
optionsdictionary:locale.matches(other, {thisRange: true, otherRange: true}). - Separate this code into multiple methods:
exactMatches,matchesThisRange,matchesRanges. - Add a flag to
LocalecalledundefinedAsRangeand a method calledtoRange()which returns a newLocaleinstance based on the one the method was called on.matchwould inspect the value of the flag for boththisandotherand act accordingly.
Now, I understand that this code is based on the existing C++/Rust code. With that in mind, I think this can be moved to a follow-up. Please file it if you agree.
2 participants
This patch brings the JS implementation very close to the C++ and Rust implementations.